CURA-13049 Add a Reusable ChainableEnvironment and optimize the EnvironmentMap#2
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a reusable chained-environment base type to share shadowing logic across environment implementations, and optimizes EnvironmentMap lookups to avoid repeated key retrieval.
Changes:
- Optimized
EnvironmentMap::get()to do a single hash lookup viafind(). - Added
ChainableEnvironmentto centralize “local + shadow environment” behavior; refactoredLocalEnvironmentto derive from it. - Added bulk
add()APIs for inserting multiple bindings intoEnvironmentMap/LocalEnvironment.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/ast/ast.cpp |
Implements ChainableEnvironment logic and updates EnvironmentMap retrieval/add paths. |
include/cura-formulae-engine/ast/ast.h |
Adds ChainableEnvironment, refactors LocalEnvironment, and exposes new add() APIs. |
conandata.yml |
Bumps package version to 1.1.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| auto scoped = local_environment_.getAll(); | ||
| auto scoped = getAllImpl(); | ||
| all.insert(scoped.begin(), scoped.end()); |
There was a problem hiding this comment.
ChainableEnvironment::getAll() builds the result by starting with the shadow environment and then calling all.insert(scoped.begin(), scoped.end()). insert does not overwrite existing keys, so any key present in the shadow will prevent the local/scoped value from being included. This contradicts get()/has() behavior where the local environment should shadow the outer one. Use an overwrite merge strategy (e.g., insert-or-assign for scoped entries, or build from scoped then add missing shadow entries).
| all.insert(scoped.begin(), scoped.end()); | |
| for (const auto& [key, value] : scoped) | |
| { | |
| all.insert_or_assign(key, value); | |
| } |
There was a problem hiding this comment.
I do agree with the suggestion here, why not use insert_or_assign to assure scoped will override the shadow env? I do realize that this was the code before, just wondering if this was ever the right logic
There was a problem hiding this comment.
I think the overriding ordering was never a real question, since the keys in each chained environment are in practice mutually exclusive. We could make it an actual topic if required, but I don't really see a need yet. That would also imply writing documentation to make it explicit.
There was a problem hiding this comment.
I disagree that keys are always mutually exclusive, I can see a ton of use cases where global stack is shadowing an extruder stack for instance and both stacks have a ton of duplicate keys. For me the naming ShadowEnvoronment makes it very much expected that the variables from this env will be overwritten.
There was a problem hiding this comment.
I totally agree that the same setting can be in both the extruder settings and the global settings, however this is handled internally in CuraEngine by having the Settings objects chained together. So in the end, when asking for a value in the extruders settings adapter, it will call the global Setting objects if the extruder Setting has no such key. However the adapter containing the global Setting is not linked to the adapter containing the extruders Settings
I added a diagram in the engine documentation to get an overview of the adapters chain:
There was a problem hiding this comment.
But then the current logic is still non-expected right and thus error-prone? I can imagine situations in either curaengine or curator where we would rely on this behavior. I believe we're currently not relying on this behavior but it would be a pain to debug.
And if we don't want the for-loop we can also adjust the ordering of both insertion-blocks.
There was a problem hiding this comment.
Yes indeed, the current logic is inconsistent and could lead to bugs in the future. I will do the changes to improve it.
CURA-13049
CURA-13049
This PR adds a few changes to help with the development of CURA-13049:
ChainableEnvironmentclassEnvironmentMapby not doing keys retrieval multiple timesCURA-13049